-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
format: airbyte-ci format fix all
on PR / airbyte-ci format check all
on master
#32491
format: airbyte-ci format fix all
on PR / airbyte-ci format check all
on master
#32491
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
airbyte-ci format fix all
on PR from airbyte-ci format check all
on masterairbyte-ci format fix all
on PR / airbyte-ci format check all
on master
@@ -1,4 +1,4 @@ | |||
name: Check for formatting errors | |||
name: Automatic Formatting on PRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're going to want to follow the "fix" with a "check". Not everything can always be fixed automatically and formatting errors should be caught before the PR is merged. In fact, that's the DoD for this whole initiative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 715a8e0
@@ -0,0 +1,59 @@ | |||
name: Check for formatting errors on head ref |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocking comment but in the near future we're going to want to subsume a larger class of checks than just formatting checks in this workflow, so its name should reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer keeping this name as it is as we don't know yet what's going to be in this "larger class of checks" and we might want to split them in multiple workflows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
e656bcb
to
715a8e0
Compare
What
I think we want two separate worfklows for
format
:master
and yells in slack if something is not formatted onmaster
How
format.yml
in two worfklowsformat_fix.yml
: Runsairbyte format fix all
on PRs + commit changesformat_check.yml
: Runsairbyte format check all
on merge to master and sends a slack message in case of failure.